-
Notifications
You must be signed in to change notification settings - Fork 84
feat(l2): implement batch endpoint #3374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Lines of code reportTotal lines added: Detailed view
|
**Description** This PR fixes a panic we had when the user didn't specify any of the `proof-coordinator.*` flags. It seems clap called the `Default::default` implementation, which panicked because of the parsing not having support for leading `0x`. Closes #3309
**Description** This PR updates the withdrawals documentation with native ERC20 withdrawals and adds documentation for L2 deposits. --------- Co-authored-by: Javier Rodríguez Chatruc <[email protected]> Co-authored-by: Avila Gastón <[email protected]> Co-authored-by: Copilot <[email protected]>
**Motivation** https://docs.ethrex.xyz/, isn't rendering LaTex expression **Description** This PR adds the [mdbook-katex](https://github.com/lzanini/mdbook-katex) preprocessor to `book.toml` to enable LaTeX rendering. Run the following command to check how the documentation looks ```bash make docs-deps && make docs-serve ``` **Before**  **After** 
**Motivation** In #3242, `verifyBatchesAligned()` was updated in the based `OnChainProposer` to be consistent with the non-based one, but the `onlySequencer` identifier was added by mistake. **Description** Removes the `onlySequencer` identifier. Closes None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements storage and retrieval of commit and verify transaction hashes for L2 batches, and exposes them via a new ethrex_getBatchByNumber
RPC endpoint. Key changes include:
- Extending the
Batch
struct withcommit_tx
andverify_tx
and updating all storage backends (SQL, Redb, LMDBX, in-memory) to persist these. - Capturing and storing verify transaction logs in the block fetcher and sequencer modules.
- Adding a new RPC handler for
ethrex_getBatchByNumber
that returns the enrichedBatch
data.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
crates/l2/storage/src/store_db/sql.rs | Added commit_txs /verify_txs tables and SQLStore methods |
crates/l2/storage/src/store_db/redb.rs | Added Redb tables and methods for commit/verify TXs |
crates/l2/storage/src/store_db/libmdbx.rs | Added LMDBX tables and methods for commit/verify TXs |
crates/l2/storage/src/store_db/in_memory.rs | Extended in-memory store with commit/verify TX maps |
crates/l2/storage/src/store.rs | Piped new methods into the Store API and get_batch logic |
crates/l2/storage/src/api.rs | Extended StoreEngineRollup trait with commit/verify TX methods |
crates/l2/sequencer/l1_proof_verifier.rs | Storing verify TX hashes for aggregated proofs |
crates/l2/sequencer/l1_proof_sender.rs | Storing verify TX after sending proof |
crates/l2/sequencer/l1_committer.rs | Storing commit TX after batch commitment |
crates/l2/networking/rpc/rpc.rs | Routed ethrex_getBatchByNumber method |
crates/l2/networking/rpc/l2/batch.rs | Implemented RPC objects and handler for batch endpoint |
crates/l2/based/block_fetcher.rs | Fetching/verifying logs and storing both TX hashes; refactored fetching logic |
crates/common/types/batch.rs | Made Batch serializable and added new fields |
Comments suppressed due to low confidence (3)
crates/l2/networking/rpc/rpc.rs:211
- Add unit or integration tests for the new
ethrex_getBatchByNumber
RPC endpoint to verify thatcommit_tx
andverify_tx
fields are correctly returned in various scenarios.
"ethrex_getBatchByNumber" => GetBatchByBatchNumberRequest::call(req, context).await,
crates/common/types/batch.rs:18
- [nitpick] Consider annotating
commit_tx
andverify_tx
with#[serde(skip_serializing_if = "Option::is_none")]
so thatnull
values are omitted from JSON when not set.
pub verify_tx: Option<H256>,
crates/l2/storage/src/store_db/in_memory.rs:42
- Ensure that
commit_txs
andverify_txs
are initialized when constructingStoreInner
(e.g., in itsnew
orDefault
impl) so they don't remain uninitialized at runtime.
commit_txs: HashMap<u64, H256>,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to implement the new method in the EthClient
state: &mut BlockFetcherState, | ||
last_l2_batch_number_known: u64, | ||
) -> Result<(), BlockFetcherError> { | ||
let mut missing_batches_logs = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider returning a BTreeMap<BatchNumber, Log> instead of a Vec with tuples if possible. This will save us the call to sorting here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is simply a refactor to also process verified_logs. I just copied and pasted what was done before. We can review this in the future.
#[serde(flatten)] | ||
pub batch: Batch, | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub block_hashes: Option<Vec<BlockHash>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If block hashes can be none, why not simply use an empty vec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The block_hashes
field is optional. You can choose whether to request them using a boolean parameter, similar to this. IMO, an empty vec could indicate that the batch is empty.
// Store the verify transaction hash for each batch that was aggregated. | ||
for i in 0..aggregated_proofs_count { | ||
let batch_number = first_batch_number + i; | ||
self.rollup_store | ||
.store_verify_tx_by_batch(batch_number, verify_tx_hash) | ||
.await?; | ||
} | ||
|
||
Ok(Some(verify_tx_hash)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to create a function that takes a range of batch numbers and stores them all in a single transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #3432!
**Motivation** For debugging purposes, it's useful to have an `ethrex_getBatchByNumber` endpoint that returns a `Batch` struct: ```Rust pub struct Batch { pub number: u64, pub first_block: u64, pub last_block: u64, pub state_root: H256, pub deposit_logs_hash: H256, pub message_hashes: Vec<H256>, pub blobs_bundle: BlobsBundle, pub commit_tx: Option<H256>, pub verify_tx: Option<H256>, } ``` **Description** - Modifies the `Batch` struct to incude `commit_tx` and `verify_tx`. - Updates `block_fetcher` to process verify tx logs and extract the verify tx hashes as well. - Fixes a bug found during development: the `rollup_storage::getBatch()` function incorrectly treated batches without `L1Messages` as an error. ## How to test You can run: ```bash curl -X POST http://localhost:1729 \ -H "Content-Type: application/json" \ -d '{ "jsonrpc":"2.0", "method":"ethrex_getBatchByNumber", "params": ["0x1", true], "id":1 }' ``` Closes None --------- Co-authored-by: Tomás Grüner <[email protected]> Co-authored-by: Javier Rodríguez Chatruc <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Damian Ramirez <[email protected]>
Motivation
For debugging purposes, it's useful to have an
ethrex_getBatchByNumber
endpoint that returns aBatch
struct:Description
Batch
struct to incudecommit_tx
andverify_tx
.block_fetcher
to process verify tx logs and extract the verify tx hashes as well.rollup_storage::getBatch()
function incorrectly treated batches withoutL1Messages
as an error.How to test
You can run:
Closes None